internal/controller/csrapproval/csr_owner_validation.go (1)

89-103: Use Get by name instead of List + linear scan.

dpuExists lists all DPUs in the namespace and iterates to find a matching name. A direct Get call is more efficient (O(1) API call vs. listing potentially many objects). Same applies to nodeExists below.

♻️ Proposed refactor for dpuExists

 func (v *Validator) dpuExists(ctx context.Context, hostname string) (bool, error) {
-	dpuList := &dpuprovisioningv1alpha1.DPUList{}
-	if err := v.mgmtClient.List(ctx, dpuList, client.InNamespace(v.dpuNamespace)); err != nil {
-		return false, err
-	}
-
-	for _, dpu := range dpuList.Items {
-		if dpu.Name == hostname {
-			return true, nil
-		}
-	}
-
-	return false, nil
+	dpu := &dpuprovisioningv1alpha1.DPU{}
+	err := v.mgmtClient.Get(ctx, client.ObjectKey{Namespace: v.dpuNamespace, Name: hostname}, dpu)
+	if err != nil {
+		if client.IgnoreNotFound(err) == nil {
+			return false, nil
+		}
+		return false, err
+	}
+	return true, nil
 }

♻️ Proposed refactor for nodeExists

 func (v *Validator) nodeExists(ctx context.Context, hostname string) (bool, error) {
-	nodeList, err := v.hostedClient.CoreV1().Nodes().List(ctx, metav1.ListOptions{})
-	if err != nil {
-		return false, err
-	}
-
-	for _, node := range nodeList.Items {
-		if node.Name == hostname {
-			return true, nil
-		}
-	}
-
-	return false, nil
+	_, err := v.hostedClient.CoreV1().Nodes().Get(ctx, hostname, metav1.GetOptions{})
+	if err != nil {
+		if errors.IsNotFound(err) {
+			return false, nil
+		}
+		return false, err
+	}
+	return true, nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/csr_owner_validation.go` around lines 89 -
103, Replace the List+scan approach in dpuExists and nodeExists with direct Get
calls: for dpuExists use v.mgmtClient.Get(ctx, types.NamespacedName{Name:
hostname, Namespace: v.dpuNamespace}, &dpuprovisioningv1alpha1.DPU{}) and return
true,nil if found, return false,nil on apierrors.IsNotFound(err) and return
false,err for other errors; for nodeExists (cluster-scoped) use
v.mgmtClient.Get(ctx, types.NamespacedName{Name: hostname}, &corev1.Node{}) with
the same NotFound/other-error handling and return true,nil when present. Ensure
you import types and apierrors if not already present.

internal/controller/csrapproval/hostname_test.go (1)

297-391: Consider consolidating duplicated CSR helper functions.

createTestCSR, createTestCSRWithOrganization, and createServingCSRWithDNS share the same key generation, CSR creation, PEM encoding, and base64 encoding boilerplate. A single parameterized helper would reduce duplication.

♻️ Proposed consolidated helper

-func createTestCSR(cn string) []byte {
-	privateKey, err := rsa.GenerateKey(rand.Reader, 2048)
-	if err != nil {
-		panic(err)
-	}
-	template := x509.CertificateRequest{
-		Subject: pkix.Name{
-			CommonName: cn,
-		},
-	}
-	csrBytes, err := x509.CreateCertificateRequest(rand.Reader, &template, privateKey)
-	if err != nil {
-		panic(err)
-	}
-	pemBlock := pem.EncodeToMemory(&pem.Block{
-		Type:  "CERTIFICATE REQUEST",
-		Bytes: csrBytes,
-	})
-	return []byte(base64.StdEncoding.EncodeToString(pemBlock))
-}
-
-func createTestCSRWithOrganization(cn, org string) []byte { ... }
-func createServingCSRWithDNS(cn, org, dnsName string) []byte { ... }
+// buildTestCSR creates a test CSR with optional organization and DNS names.
+func buildTestCSR(cn string, org string, dnsNames ...string) []byte {
+	privateKey, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		panic(err)
+	}
+	template := x509.CertificateRequest{
+		Subject: pkix.Name{
+			CommonName: cn,
+		},
+		DNSNames: dnsNames,
+	}
+	if org != "" {
+		template.Subject.Organization = []string{org}
+	}
+	csrBytes, err := x509.CreateCertificateRequest(rand.Reader, &template, privateKey)
+	if err != nil {
+		panic(err)
+	}
+	pemBlock := pem.EncodeToMemory(&pem.Block{
+		Type:  "CERTIFICATE REQUEST",
+		Bytes: csrBytes,
+	})
+	return []byte(base64.StdEncoding.EncodeToString(pemBlock))
+}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/hostname_test.go` around lines 297 - 391, The
three helpers createTestCSR, createTestCSRWithOrganization, and
createServingCSRWithDNS duplicate key generation, CSR creation, PEM encoding,
and base64 encoding; replace them with a single parameterized helper (e.g.,
buildCSR(cn string, orgs []string, dnsNames []string) []byte) that generates the
RSA key, constructs an x509.CertificateRequest using Subject.CommonName and
Subject.Organization from orgs, sets DNSNames from dnsNames, creates the CSR,
PEM-encodes it and returns base64 bytes; update or remove the three original
functions to call or forward to buildCSR to eliminate duplication.

internal/controller/csrapproval/client.go (1)

50-68: Cached client is never invalidated on error — stale connections will persist.

When a cached clientset becomes stale (e.g., kubeconfig rotated, API server restarted), GetHostedClusterClient keeps returning it without validation. InvalidateClient exists but nothing calls it on connection failures. TestConnection is defined but never invoked from within GetHostedClusterClient.

Consider invalidating the cache entry when the caller encounters connection errors, or proactively test the cached client before returning it.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/client.go` around lines 50 - 68,
GetHostedClusterClient currently returns cached entries from cm.hcClients
without validating them; call the existing TestConnection method on the cached
kubernetes.Clientset (keyed by namespace+"/"+name) before returning and if
TestConnection returns an error, call InvalidateClient for that key and fall
through to createHostedClusterClient to rebuild the client; ensure
createHostedClusterClient results are only cached on successful creation and
that errors from TestConnection lead to recreating the client instead of
returning a stale one.

internal/controller/csrapproval/csr_owner_validation.go (2)

119-133: Same improvement applies to nodeExists — use a direct Get instead of listing all nodes.

Listing all nodes in the hosted cluster on every CSR is expensive, especially as clusters grow. A direct Get by name is more efficient:

♻️ Proposed refactor

 func (v *Validator) nodeExists(ctx context.Context, hostname string) (bool, error) {
-	nodeList, err := v.hostedClient.CoreV1().Nodes().List(ctx, metav1.ListOptions{})
-	if err != nil {
-		return false, err
-	}
-
-	for _, node := range nodeList.Items {
-		if node.Name == hostname {
-			return true, nil
-		}
-	}
-
-	return false, nil
+	_, err := v.hostedClient.CoreV1().Nodes().Get(ctx, hostname, metav1.GetOptions{})
+	if err != nil {
+		if apierrors.IsNotFound(err) {
+			return false, nil
+		}
+		return false, err
+	}
+	return true, nil
 }

You'll need to import apierrors "k8s.io/apimachinery/pkg/api/errors".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/csr_owner_validation.go` around lines 119 -
133, Replace the expensive list in nodeExists with a direct node Get: call
v.hostedClient.CoreV1().Nodes().Get(ctx, hostname, metav1.GetOptions{}) (in
function nodeExists) and handle errors using apierrors.IsNotFound(err) to return
(false, nil) when the node doesn't exist, otherwise return (false, err) for
other errors; add the import apierrors "k8s.io/apimachinery/pkg/api/errors".

---

104-117: Consider using a direct Get instead of listing all DPUs.

dpuExists lists every DPU in the namespace and iterates to find a name match. Since you're matching by dpu.Name == hostname, a direct Get by name would be O(1) vs O(n) and avoids transferring the full list:

♻️ Proposed refactor

 func (v *Validator) dpuExists(ctx context.Context, hostname string) (bool, error) {
-	dpuList := &dpuprovisioningv1alpha1.DPUList{}
-	if err := v.mgmtClient.List(ctx, dpuList, client.InNamespace(v.dpuNamespace)); err != nil {
-		return false, err
-	}
-
-	for _, dpu := range dpuList.Items {
-		if dpu.Name == hostname {
-			return true, nil
-		}
-	}
-
-	return false, nil
+	dpu := &dpuprovisioningv1alpha1.DPU{}
+	key := client.ObjectKey{Namespace: v.dpuNamespace, Name: hostname}
+	if err := v.mgmtClient.Get(ctx, key, dpu); err != nil {
+		if client.IgnoreNotFound(err) == nil {
+			return false, nil
+		}
+		return false, err
+	}
+	return true, nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/csr_owner_validation.go` around lines 104 -
117, Replace the List+loop in Validator.dpuExists with a direct Get by
namespaced name: construct a dpuprovisioningv1alpha1.DPU object and call
v.mgmtClient.Get(ctx, types.NamespacedName{Name: hostname, Namespace:
v.dpuNamespace}, &dpu); if Get returns nil return (true, nil), if it returns a
NotFound error (use k8s.io/apimachinery/pkg/api/errors.IsNotFound) return
(false, nil), otherwise return (false, err). This avoids iterating over dpuList
and keeps the function signature and return semantics the same.

internal/controller/csrapproval/csrapproval.go (1)

260-261: A new Validator is instantiated per CSR — consider reusing it across the batch.

NewValidator is created inside processCSR, which is called for every pending CSR. The Validator struct is lightweight, but it triggers list operations (dpuExists, nodeExists) that query the API for each CSR. Creating the Validator once in processPendingCSRs and passing it to processCSR would allow potential future optimizations (e.g., caching the DPU list once per polling cycle).

♻️ Proposed refactor sketch

 // In processPendingCSRs, create the validator once:
+	validator := NewValidator(a.mgmtClient, hcClient, dpfhcp.Spec.DPUClusterRef.Namespace)
 
 // Then pass it to processCSR:
-func (a *CSRApprover) processCSR(ctx context.Context, dpfhcp *provisioningv1alpha1.DPFHCPProvisioner, hcClient *kubernetes.Clientset, csr *certv1.CertificateSigningRequest, csrType string) error {
+func (a *CSRApprover) processCSR(ctx context.Context, dpfhcp *provisioningv1alpha1.DPFHCPProvisioner, hcClient *kubernetes.Clientset, csr *certv1.CertificateSigningRequest, csrType string, validator *Validator) error {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/csrapproval.go` around lines 260 - 261, The
Validator is being created per CSR inside processCSR (via NewValidator), causing
repeated list queries; instead, create a single Validator in processPendingCSRs
and pass it into processCSR as a parameter (e.g., add a *Validator arg to
processCSR and remove the NewValidator call inside it). Update all callers of
processCSR accordingly, and ensure the existing methods on Validator (dpuExists,
nodeExists) are used from the shared instance so future caching or batched list
optimizations can be added centrally.

internal/controller/csrapproval/csr_content_validation.go (1)

162-208: Consider extracting a generic usage validation helper to reduce duplication.

validateBootstrapUsages and validateServingUsages are nearly identical, differing only in the required usage set. A single validateRequiredUsages(csr, required) helper would eliminate the duplication.

♻️ Proposed refactor

+func validateRequiredUsages(csr *certv1.CertificateSigningRequest, required []certv1.KeyUsage) error {
+	usageSet := make(map[certv1.KeyUsage]bool, len(required))
+	for _, u := range required {
+		usageSet[u] = false
+	}
+	for _, usage := range csr.Spec.Usages {
+		if _, ok := usageSet[usage]; ok {
+			usageSet[usage] = true
+		}
+	}
+	for usage, found := range usageSet {
+		if !found {
+			return fmt.Errorf("missing required usage: %s", usage)
+		}
+	}
+	return nil
+}
+
 func validateBootstrapUsages(csr *certv1.CertificateSigningRequest) error {
-	requiredUsages := map[certv1.KeyUsage]bool{...}
-	...
+	return validateRequiredUsages(csr, []certv1.KeyUsage{
+		certv1.UsageDigitalSignature,
+		certv1.UsageClientAuth,
+	})
 }
 
 func validateServingUsages(csr *certv1.CertificateSigningRequest) error {
-	requiredUsages := map[certv1.KeyUsage]bool{...}
-	...
+	return validateRequiredUsages(csr, []certv1.KeyUsage{
+		certv1.UsageDigitalSignature,
+		certv1.UsageServerAuth,
+	})
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/csr_content_validation.go` around lines 162 -
208, Extract the duplicated logic in validateBootstrapUsages and
validateServingUsages into a single helper validateRequiredUsages that takes the
CSR and the set/list of required usages (e.g. func validateRequiredUsages(csr
*certv1.CertificateSigningRequest, required []certv1.KeyUsage) error or a map),
move the loop that marks found usages and the final missing-check into that
helper, and then have validateBootstrapUsages and validateServingUsages call
validateRequiredUsages with their respective required usage sets
(UsageDigitalSignature+UsageClientAuth and
UsageDigitalSignature+UsageServerAuth) so behavior and error messages
(fmt.Errorf("missing required usage: %s", usage)) remain the same.

internal/controller/csrapproval/controller.go (1)

34-39: Scheme and Recorder fields are unused in this controller.

Scheme and Recorder are set during wiring in main.go but never referenced inside Reconcile or SetupWithManager. If they exist only for structural consistency with the main reconciler, consider documenting that intent; otherwise, removing them avoids confusion.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/controller.go` around lines 34 - 39, The
CSRApprovalReconciler struct contains unused fields Scheme and Recorder; either
remove these fields from CSRApprovalReconciler or explicitly document why
they’re retained (e.g., wiring consistency with main.go) and reference their
intended use. Locate the CSRApprovalReconciler type definition and delete Scheme
*runtime.Scheme and Recorder record.EventRecorder if they are not referenced by
Reconcile or SetupWithManager, or add a comment above the struct explaining they
are intentionally kept for wiring in main.go and will be used in future logic;
update any constructor/wiring code in main.go accordingly to avoid linter
confusion.

internal/controller/csrapproval/csr_owner_validation.go (3)

88-100: Drop the unnecessary else after early return — idiomatic Go.

The else block on line 88 is unreachable in any other sense; the prior if already returns, so the else clause adds indentation noise and is non-idiomatic Go.

♻️ Proposed refactor

-	} else {
-		// Serving CSR: node SHOULD exist (already joined via bootstrap CSR)
-		if !nodeExists {
-			return &ValidationResult{
-				Valid:  false,
-				Reason: fmt.Sprintf("node %s does not exist yet in hosted cluster", hostname),
-			}, nil
-		}
-		return &ValidationResult{
-			Valid:  true,
-			Reason: "DPU exists and node already joined",
-		}, nil
-	}
+	// Serving CSR: node SHOULD exist (already joined via bootstrap CSR)
+	if !nodeExists {
+		return &ValidationResult{
+			Valid:  false,
+			Reason: fmt.Sprintf("node %s does not exist yet in hosted cluster", hostname),
+		}, nil
+	}
+	return &ValidationResult{
+		Valid:  true,
+		Reason: "DPU exists and node already joined",
+	}, nil

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/csr_owner_validation.go` around lines 88 -
100, Remove the unnecessary else block following the early return: in the
function that constructs a ValidationResult using variables nodeExists and
hostname (the block that currently returns fmt.Sprintf("node %s does not exist
yet in hosted cluster", hostname) inside an else), delete the else and outdent
its contents so the nodeExists check and the two ValidationResult returns (the
"node ... does not exist yet in hosted cluster" and "DPU exists and node already
joined") become the natural fall-through after the prior return; keep the same
return values and use the same ValidationResult struct.

---

56-101: No unit tests for ValidateCSROwner — security-critical path lacks coverage.

The related files include csr_content_validation_test.go and hostname_test.go, but no csr_owner_validation_test.go appears in this PR. Given that ValidateCSROwner is the gatekeeper deciding whether a CSR gets auto-approved, the following cases should be covered at minimum:

• DPU missing → invalid result
• Bootstrap CSR, node absent → valid
• Bootstrap CSR, node present → invalid
• Serving CSR, node present → valid
• Serving CSR, node absent → invalid
• dpuExists/nodeExists errors propagate correctly

Would you like me to generate a csr_owner_validation_test.go covering these cases using the controller-runtime fake client and a fake kubernetes.Interface? I can open a new issue to track this as well.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/csr_owner_validation.go` around lines 56 -
101, Add unit tests for ValidateCSROwner covering the six scenarios listed: DPU
missing returns Valid=false with the expected Reason; bootstrap CSR with node
absent returns Valid=true; bootstrap CSR with node present returns Valid=false;
serving CSR with node present returns Valid=true; serving CSR with node absent
returns Valid=false; and both dpuExists/nodeExists returning errors propagate
the error. Create csr_owner_validation_test.go and use controller-runtime's fake
client to control DPU existence and a fake kubernetes.Interface (or a client
stub) to control nodeExists behavior; call ValidateCSROwner and assert the
ValidationResult.Valid and Reason values for each case and that errors from
dpuExists/nodeExists are returned (not swallowed). Ensure tests reference the
Validator type and its methods dpuExists and nodeExists (mock/stub them or
inject a test-friendly interface) so behavior is deterministic.

---

104-133: Both helpers do an O(n) full-list scan; prefer a direct point-lookup instead.

dpuExists lists every DPU in the namespace and nodeExists lists every Node in the hosted cluster, both only to match a single name. A direct Get is O(1), avoids transferring the full list over the wire, and is simpler to read. Since DPU names map 1:1 to hostnames, there is no ambiguity in the lookup key.

♻️ Proposed refactor

Add apierrors "k8s.io/apimachinery/pkg/api/errors" to the import block, then replace both helpers:

+import (
+	"context"
+	"fmt"
+
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/kubernetes"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	dpuprovisioningv1alpha1 "github.com/nvidia/doca-platform/api/provisioning/v1alpha1"
+)

 func (v *Validator) dpuExists(ctx context.Context, hostname string) (bool, error) {
-	dpuList := &dpuprovisioningv1alpha1.DPUList{}
-	if err := v.mgmtClient.List(ctx, dpuList, client.InNamespace(v.dpuNamespace)); err != nil {
-		return false, err
-	}
-
-	for _, dpu := range dpuList.Items {
-		if dpu.Name == hostname {
-			return true, nil
-		}
-	}
-
-	return false, nil
+	dpu := &dpuprovisioningv1alpha1.DPU{}
+	err := v.mgmtClient.Get(ctx, types.NamespacedName{Name: hostname, Namespace: v.dpuNamespace}, dpu)
+	if err != nil {
+		if apierrors.IsNotFound(err) {
+			return false, nil
+		}
+		return false, err
+	}
+	return true, nil
 }

 func (v *Validator) nodeExists(ctx context.Context, hostname string) (bool, error) {
-	nodeList, err := v.hostedClient.CoreV1().Nodes().List(ctx, metav1.ListOptions{})
+	_, err := v.hostedClient.CoreV1().Nodes().Get(ctx, hostname, metav1.GetOptions{})
 	if err != nil {
-		return false, err
-	}
-
-	for _, node := range nodeList.Items {
-		if node.Name == hostname {
-			return true, nil
+		if apierrors.IsNotFound(err) {
+			return false, nil
 		}
+		return false, err
 	}
-
-	return false, nil
+	return true, nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/csr_owner_validation.go` around lines 104 -
133, Replace the full-list scans in dpuExists and nodeExists with direct Get
calls: import k8s apierrors and call v.mgmtClient.Get(ctx,
client.ObjectKey{Namespace: v.dpuNamespace, Name: hostname},
&dpuprovisioningv1alpha1.DPU{}) in dpuExists and use
v.hostedClient.CoreV1().Nodes().Get(ctx, hostname, metav1.GetOptions{}) in
nodeExists; treat apierrors.IsNotFound as a false/no-error result and return
other errors upward so you avoid O(n) list operations and return correct
boolean+error behavior for both helpers.

internal/controller/csrapproval/client.go (1)

159-191: replaceServerWithInternalEndpoint has two unused parameters and a package-name shadow.

1. ctx context.Context and mgmtClient client.Client are accepted as parameters but never referenced in the function body — they are dead parameters.
2. context, ok := kubeconfig.Contexts[currentContext] (Line 170) declares a local variable named context that shadows the imported "context" standard library package within this function's scope. Rename it to kubeconfigCtx or ctxEntry to avoid future confusion.

♻️ Proposed cleanup

-func replaceServerWithInternalEndpoint(ctx context.Context, mgmtClient client.Client, kubeconfig *clientcmdapi.Config, hostedClusterNamespace, hostedClusterName string) error {
+func replaceServerWithInternalEndpoint(kubeconfig *clientcmdapi.Config, hostedClusterNamespace, hostedClusterName string) error {
 	if kubeconfig == nil {
 		return fmt.Errorf("kubeconfig is nil")
 	}

 	currentContext := kubeconfig.CurrentContext
 	if currentContext == "" {
 		return fmt.Errorf("kubeconfig has no current context")
 	}

-	context, ok := kubeconfig.Contexts[currentContext]
+	kubeconfigCtx, ok := kubeconfig.Contexts[currentContext]
 	if !ok {
 		return fmt.Errorf("context %s not found in kubeconfig", currentContext)
 	}

-	clusterName := context.Cluster
+	clusterName := kubeconfigCtx.Cluster
 	...
 }

And update the call site in createHostedClusterClient:

-	if err := replaceServerWithInternalEndpoint(ctx, cm.mgmtClient, kubeconfig, namespace, name); err != nil {
+	if err := replaceServerWithInternalEndpoint(kubeconfig, namespace, name); err != nil {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/client.go` around lines 159 - 191, The
function replaceServerWithInternalEndpoint currently accepts unused parameters
ctx and mgmtClient and declares a local variable named context that shadows the
imported context package; remove the unused parameters from
replaceServerWithInternalEndpoint's signature and update its callers (e.g.,
createHostedClusterClient) to match the new signature, and rename the local
variable context (e.g., to kubeconfigCtx or ctxEntry) where
kubeconfig.Contexts[currentContext] is read to avoid shadowing the standard
library package.

api/v1alpha1/dpfhcpprovisioner_types.go (1)

238-239: Consider a more descriptive value for ReasonCSRApprovalActive.

The value "Active" is ambiguous in isolation — kubectl get dpfhcp -o yaml will show only the Reason field without the condition type context. Every other reason constant in this file uses a self-describing string (e.g., "AllComponentsOperational", "HostedClusterNotReady"). A value like "CSRApprovalActive" or "ApprovalControllerRunning" would be consistent with that convention.

✏️ Proposed rename

-	ReasonCSRApprovalActive string = "Active"
+	ReasonCSRApprovalActive string = "CSRApprovalActive"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@api/v1alpha1/dpfhcpprovisioner_types.go` around lines 238 - 239, The constant
ReasonCSRApprovalActive currently uses the ambiguous value "Active"; change its
string value to a more descriptive identifier such as "CSRApprovalActive" (i.e.,
keep the constant name ReasonCSRApprovalActive but set its value to
"CSRApprovalActive"), and update any usages/tests/fixtures that assert on the
prior "Active" literal (search for ReasonCSRApprovalActive and any string
comparisons to "Active") so output in kubectl shows the self-describing reason.

internal/controller/csrapproval/csrapproval.go (1)

264-266: NewValidator allocated on every CSR — consider hoisting to processPendingCSRs.

All three constructor arguments (a.mgmtClient, hcClient, dpfhcp.Spec.DPUClusterRef.Namespace) are invariant across the entire CSR batch. Creating one Validator per CSR is functionally harmless but wasteful. Moving creation to processPendingCSRs and threading it into processCSR (or making it a field) would be cleaner.

♻️ Proposed refactor

-// processCSR processes a single CSR (validate and approve if valid)
-func (a *CSRApprover) processCSR(ctx context.Context, dpfhcp *provisioningv1alpha1.DPFHCPProvisioner, hcClient *kubernetes.Clientset, csr *certv1.CertificateSigningRequest, csrType string) error {
+// processCSR processes a single CSR (validate and approve if valid)
+func (a *CSRApprover) processCSR(ctx context.Context, validator *Validator, hcClient *kubernetes.Clientset, csr *certv1.CertificateSigningRequest, csrType string, dpfhcp *provisioningv1alpha1.DPFHCPProvisioner) error {

And in processPendingCSRs, create the validator once before the loops:

+	validator := NewValidator(a.mgmtClient, hcClient, dpfhcp.Spec.DPUClusterRef.Namespace)
+
 	// Process bootstrap CSRs
 	for _, csr := range bootstrapCSRs {
-		if err := a.processCSR(ctx, dpfhcp, hcClient, &csr, "bootstrap"); err != nil {
+		if err := a.processCSR(ctx, validator, hcClient, &csr, "bootstrap", dpfhcp); err != nil {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/csrapproval.go` around lines 264 - 266,
NewValidator is being allocated per CSR; instead, create the Validator once in
processPendingCSRs using the invariant arguments (a.mgmtClient, hcClient,
dpfhcp.Spec.DPUClusterRef.Namespace) and pass that single instance into
processCSR (or store it as a field on the approver struct) so processCSR calls
validator.ValidateCSROwner(ctx, hostname, isBootstrapCSR) without re-allocating;
update function signatures (processCSR) or the approver struct accordingly and
remove the per-CSR NewValidator call.

internal/controller/csrapproval/csrapproval.go (2)

148-220: processPendingCSRs swallows individual CSR processing errors — consider aggregating.

Lines 202-206 and 212-216 log errors from processCSR but continue and return nil from processPendingCSRs. If all CSRs fail, the caller at Line 104 sees no error and reports "CSR auto-approval active" — which is technically true but masks systemic issues. Consider returning an aggregated error (or a count of failures) so the condition message reflects the true state.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/csrapproval.go` around lines 148 - 220,
processPendingCSRs currently logs per-CSR errors from processCSR but always
returns nil, masking systemic failures; modify processPendingCSRs to track
failures (e.g., failureCount or collect error messages) while iterating over
bootstrapCSRs and servingCSRs and after both loops return a non-nil aggregated
error (or fmt.Errorf with the failure count and brief details) when any CSR
processing failed; ensure you update the return to include context (reference
processPendingCSRs and processCSR) and keep per-CSR log calls but also surface a
summary error to the caller so the caller can detect and report when
auto-approval failed for all/part of the CSRs.

---

248-262: NewValidator is instantiated per-CSR; hoist it out of the loop.

mgmtClient, hcClient, and dpuNamespace don't change between CSRs in the same poll cycle. Creating a Validator (and its embedded clients) for every CSR is wasteful. Create it once in processPendingCSRs and pass it into processCSR.

♻️ Proposed fix

In processPendingCSRs, create the validator once and pass it to processCSR:

 func (a *CSRApprover) processPendingCSRs(ctx context.Context, dpfhcp *provisioningv1alpha1.DPFHCPProvisioner, hcClient *kubernetes.Clientset) error {
 	log := logf.FromContext(ctx)
+	validator := NewValidator(a.mgmtClient, hcClient, dpfhcp.Spec.DPUClusterRef.Namespace)
 
 	// ... (existing code) ...
 
 	for _, csr := range bootstrapCSRs {
-		_, err := a.processCSR(ctx, dpfhcp, hcClient, &csr, "bootstrap")
+		_, err := a.processCSR(ctx, dpfhcp, hcClient, &csr, "bootstrap", validator)

Then in processCSR, accept and use the pre-built validator:

-func (a *CSRApprover) processCSR(ctx context.Context, dpfhcp *provisioningv1alpha1.DPFHCPProvisioner, hcClient *kubernetes.Clientset, csr *certv1.CertificateSigningRequest, csrType string) (bool, error) {
+func (a *CSRApprover) processCSR(ctx context.Context, dpfhcp *provisioningv1alpha1.DPFHCPProvisioner, hcClient *kubernetes.Clientset, csr *certv1.CertificateSigningRequest, csrType string, validator *Validator) (bool, error) {
 	// ...
-	validator := NewValidator(a.mgmtClient, hcClient, dpfhcp.Spec.DPUClusterRef.Namespace)
 	isBootstrapCSR := csr.Spec.SignerName == SignerBootstrap

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/csrapproval.go` around lines 248 - 262,
NewValidator is being created inside processCSR for every CSR which is wasteful
because mgmtClient, hcClient and dpfhcp.Spec.DPUClusterRef.Namespace are
constant per poll; instead instantiate validator once in processPendingCSRs
(using mgmtClient, hcClient and the DPU namespace), then change processCSR to
accept a Validator parameter and remove the NewValidator call inside processCSR;
update all calls to processCSR to pass the prebuilt validator and ensure
processCSR uses the injected Validator (ValidateCSROwner) rather than creating
its own.

internal/controller/csrapproval/csr_content_validation.go (1)

108-166: Minor style inconsistency between validateBootstrapperIdentity and validateNodeIdentity group-checking logic.

validateBootstrapperIdentity uses a map[string]bool pattern to check required groups (Lines 116-133), while validateNodeIdentity uses individual boolean flags (Lines 147-163). Both are correct, but the inconsistency slightly hurts readability. Consider unifying to one pattern.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/csr_content_validation.go` around lines 108 -
166, Unify the group-checking pattern by changing validateNodeIdentity to use
the same map[string]bool approach as validateBootstrapperIdentity: keep the
username check (expectedUsername := SystemNodePrefix + hostname) but replace the
hasNodesGroup/hasAuthenticatedGroup booleans with a requiredGroups map
initialized with groupNodes and groupAuthenticated set to false, iterate
csr.Spec.Groups to mark found entries, then loop over requiredGroups to return
an error if any remain false; reference validateNodeIdentity,
validateBootstrapperIdentity, groupNodes and groupAuthenticated when making the
change.

internal/controller/csrapproval/client.go (2)

56-79: TOCTOU in GetHostedClusterClient — benign but may cause duplicate client creation.

Two concurrent goroutines can both miss the read-lock cache check (Lines 60-65), both create a client (Line 68), and both insert into the map (Lines 74-76) — the second write silently overwrites the first. The discarded client leaks its underlying transport connections until GC.

This is a minor concern since it only happens on the first access for a given key and the impact is a single wasted client creation. If you want to tighten it, a double-check after acquiring the write lock would suffice.

♻️ Double-check pattern

 	// Cache the client with write lock
 	cm.mu.Lock()
+	// Double-check: another goroutine may have inserted while we were creating
+	if existing, ok := cm.hcClients[key]; ok {
+		cm.mu.Unlock()
+		return existing, nil
+	}
 	cm.hcClients[key] = clientset
 	cm.mu.Unlock()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/client.go` around lines 56 - 79,
GetHostedClusterClient has a TOCTOU where two goroutines can both create a
client and one overwrites the other; after creating the new client via
createHostedClusterClient, acquire cm.mu.Lock() and re-check cm.hcClients[key] —
if an entry now exists, release the lock and return the existing client,
otherwise store the newly created client and return it; if you need to avoid
leaking the discarded client's connections, call a small cleanup helper (e.g.,
closeClient(newClient)) when you detect an existing entry.

---

172-205: Remove unused ctx and mgmtClient parameters from replaceServerWithInternalEndpoint.

The function signature accepts ctx context.Context and mgmtClient client.Client but neither is used in the function body. Removing them reduces noise and eliminates confusion about unnecessary dependencies.

♻️ Proposed fix

-func replaceServerWithInternalEndpoint(ctx context.Context, mgmtClient client.Client, kubeconfig *clientcmdapi.Config, hostedClusterNamespace, hostedClusterName string) error {
+func replaceServerWithInternalEndpoint(kubeconfig *clientcmdapi.Config, hostedClusterNamespace, hostedClusterName string) error {

Update the call site at line 107:

-	if err := replaceServerWithInternalEndpoint(ctx, cm.mgmtClient, kubeconfig, namespace, name); err != nil {
+	if err := replaceServerWithInternalEndpoint(kubeconfig, namespace, name); err != nil {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/client.go` around lines 172 - 205, The
function replaceServerWithInternalEndpoint currently declares unused parameters
ctx and mgmtClient; remove these parameters from the signature so it becomes
replaceServerWithInternalEndpoint(kubeconfig *clientcmdapi.Config,
hostedClusterNamespace, hostedClusterName string) error, update the function
body unchanged, and then update every call site that invokes
replaceServerWithInternalEndpoint to stop passing a context and client (pass
only the kubeconfig, hostedClusterNamespace and hostedClusterName). Also adjust
any unit tests or helper wrappers that reference this function signature to
match the new parameter list.

internal/controller/csrapproval/csr_owner_validation.go (1)

98-128: Replace List + iteration with direct Get lookups for better performance.

Both helpers list all objects then iterate to find by name. Use client.Get() for dpuExists and Nodes().Get() for nodeExists instead—these are O(1) indexed lookups at the API server versus linear scans, which matters when processing multiple CSRs per poll cycle in clusters with many DPUs/nodes.

♻️ Proposed fix

Add apierrors to imports:

import (
	"context"
	"fmt"

	apierrors "k8s.io/apimachinery/pkg/api/errors"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	"k8s.io/client-go/kubernetes"
	"sigs.k8s.io/controller-runtime/pkg/client"

	dpuprovisioningv1alpha1 "github.com/nvidia/doca-platform/api/provisioning/v1alpha1"
)

Then update the functions:

 func (v *Validator) dpuExists(ctx context.Context, hostname string) (bool, error) {
-	dpuList := &dpuprovisioningv1alpha1.DPUList{}
-	if err := v.mgmtClient.List(ctx, dpuList, client.InNamespace(v.dpuNamespace)); err != nil {
-		return false, err
-	}
-
-	for _, dpu := range dpuList.Items {
-		if dpu.Name == hostname {
-			return true, nil
-		}
+	dpu := &dpuprovisioningv1alpha1.DPU{}
+	err := v.mgmtClient.Get(ctx, client.ObjectKey{Namespace: v.dpuNamespace, Name: hostname}, dpu)
+	if err != nil {
+		if client.IgnoreNotFound(err) == nil {
+			return false, nil
+		}
+		return false, err
 	}
-
-	return false, nil
+	return true, nil
 }

 func (v *Validator) nodeExists(ctx context.Context, hostname string) (bool, error) {
-	nodeList, err := v.hostedClient.CoreV1().Nodes().List(ctx, metav1.ListOptions{})
+	_, err := v.hostedClient.CoreV1().Nodes().Get(ctx, hostname, metav1.GetOptions{})
 	if err != nil {
+		if apierrors.IsNotFound(err) {
+			return false, nil
+		}
 		return false, err
 	}
-
-	for _, node := range nodeList.Items {
-		if node.Name == hostname {
-			return true, nil
-		}
-	}
-
-	return false, nil
+	return true, nil
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/csr_owner_validation.go` around lines 98 -
128, Replace the inefficient list+iterate logic in dpuExists and nodeExists with
direct GETs: use v.mgmtClient.Get(ctx, types.NamespacedName{Name: hostname,
Namespace: v.dpuNamespace}, &dpuprovisioningv1alpha1.DPU{}) in dpuExists and
v.hostedClient.CoreV1().Nodes().Get(ctx, hostname, metav1.GetOptions{}) in
nodeExists, handle not-found using apierrors.IsNotFound to return (false, nil)
and return other errors unchanged; add the apierrors import
(k8s.io/apimachinery/pkg/api/errors) and reference the existing v.mgmtClient,
v.dpuNamespace, and v.hostedClient symbols when making these changes.

internal/controller/csrapproval/csr_content_validation_test.go (2)

180-327: Add a test case for ValidateServingCSR invalid organization (step 4 is untested).

ValidateServingCSR calls validateOrganization(certReq) (step 4), but all five ValidateServingCSR test cases use a correct "system:nodes" organization. An invalid organization would slip through undetected if the validation were regressed.

✅ Proposed additional test case

+		Context("When CSR has invalid organization", func() {
+			It("should fail validation", func() {
+				hostname := "dpu-worker-01"
+				// DNS helper still uses the right hostname, but org is wrong
+				csrBytes := createServingCSRWithDNS("system:node:"+hostname, "wrong-org", hostname)
+
+				csr := &certv1.CertificateSigningRequest{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-serving-invalid-org",
+					},
+					Spec: certv1.CertificateSigningRequestSpec{
+						SignerName: SignerServing,
+						Username:   "system:node:" + hostname,
+						Groups: []string{
+							groupNodes,
+							groupAuthenticated,
+						},
+						Usages: []certv1.KeyUsage{
+							certv1.UsageDigitalSignature,
+							certv1.UsageServerAuth,
+						},
+						Request: csrBytes,
+					},
+				}
+
+				err := ValidateServingCSR(csr, hostname)
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(ContainSubstring("invalid organization"))
+			})
+		})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/csr_content_validation_test.go` around lines
180 - 327, The tests for ValidateServingCSR are missing a case that asserts
validateOrganization(certReq) fails when the CSR organization is not
"system:nodes"; add a new spec similar to the other Context blocks that builds a
CSR with createServingCSRWithDNS using a non-matching organization (e.g.
"wrong-org"), sets Spec.Username to "system:node:"+hostname and Groups to
include/not include as needed, calls ValidateServingCSR(csr, hostname) and
expects an error containing a message about invalid organization (or the exact
substring produced by validateOrganization); use a descriptive test name like
"When CSR has invalid organization" to locate it alongside the other Contexts
and reference ValidateServingCSR and validateOrganization in the test for
clarity.

---

27-177: Add a test case for ValidateBootstrapCSR CN/hostname mismatch (step 5 is untested).

The implementation's step 5 — validateCN(certReq, hostname) — has no corresponding failure test. Every other validation step (identity, usages, org) has a negative test, but a CSR whose CN does not match the expected hostname will silently pass in this test suite if the CN validation were ever broken.

✅ Proposed additional test case

+		Context("When CSR has CN that does not match the hostname", func() {
+			It("should fail validation", func() {
+				hostname := "dpu-worker-01"
+				// Create CSR bytes with a different CN
+				csrBytes := createTestCSRWithOrganization("system:node:other-node", "system:nodes")
+
+				csr := &certv1.CertificateSigningRequest{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-cn-mismatch",
+					},
+					Spec: certv1.CertificateSigningRequestSpec{
+						SignerName: SignerBootstrap,
+						Username:   BootstrapperUsername,
+						Groups: []string{
+							groupServiceAccounts,
+							groupServiceAccountsMachineConfig,
+							groupAuthenticated,
+						},
+						Usages: []certv1.KeyUsage{
+							certv1.UsageDigitalSignature,
+							certv1.UsageClientAuth,
+						},
+						Request: csrBytes,
+					},
+				}
+
+				err := ValidateBootstrapCSR(csr, hostname)
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(ContainSubstring("CN validation failed"))
+			})
+		})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/csr_content_validation_test.go` around lines
27 - 177, Add a negative test that covers step 5 (CN/hostname validation) by
creating a CSR whose organization and other fields are valid but whose common
name (CN) does not match the provided hostname; call ValidateBootstrapCSR(csr,
hostname) and assert it returns an error and that err.Error() contains a
substring indicating the CN/hostname mismatch (e.g., "CN" or "common name");
place this new test as a Context similar to the other negative cases and reuse
helpers like createTestCSRWithOrganization and constants such as SignerBootstrap
and BootstrapperUsername so it exercises validateCN(certReq, hostname).

internal/controller/csrapproval/hostname_test.go (1)

296-390: Consider extracting common CSR generation logic to reduce duplication.

The three helper functions (createTestCSR, createTestCSRWithOrganization, createServingCSRWithDNS) share nearly identical key generation, CSR creation, and PEM encoding logic. A single parameterized helper would reduce the ~90 lines of duplication.

♻️ Example consolidation

+// createTestCSRRaw creates a test CSR with the given template
+func createTestCSRRaw(template x509.CertificateRequest) []byte {
+	privateKey, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		panic(err)
+	}
+	csrBytes, err := x509.CreateCertificateRequest(rand.Reader, &template, privateKey)
+	if err != nil {
+		panic(err)
+	}
+	return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csrBytes})
+}
+
 func createTestCSR(cn string) []byte {
-	privateKey, err := rsa.GenerateKey(rand.Reader, 2048)
-	if err != nil {
-		panic(err)
-	}
-	template := x509.CertificateRequest{
+	return createTestCSRRaw(x509.CertificateRequest{
 		Subject: pkix.Name{CommonName: cn},
-	}
-	csrBytes, err := x509.CreateCertificateRequest(rand.Reader, &template, privateKey)
-	if err != nil {
-		panic(err)
-	}
-	pemBlock := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csrBytes})
-	return pemBlock
+	})
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/hostname_test.go` around lines 296 - 390, The
three helpers createTestCSR, createTestCSRWithOrganization, and
createServingCSRWithDNS duplicate key generation, x509.CreateCertificateRequest
and PEM encoding; consolidate them by adding a single helper (e.g., new
createCSR helper) that takes parameters for commonName (cn string),
organizations ([]string) and dnsNames ([]string) and performs rsa.GenerateKey,
x509.CreateCertificateRequest and pem.EncodeToMemory once, returning []byte;
then change createTestCSR, createTestCSRWithOrganization and
createServingCSRWithDNS to call createCSR with the appropriate args (nil/empty
slices where not needed) so the duplicated logic is removed while keeping the
same panic-on-error behavior for tests.

internal/controller/csrapproval/client.go (2)

56-79: Minor TOCTOU in double-checked locking — two goroutines can create duplicate clients for the same key.

Between RUnlock() at Line 65 and Lock() at Line 74, a second goroutine can also miss the cache and create a redundant client. The extra client is silently overwritten and GC'd, so this isn't a correctness issue, but it wastes a kubeconfig parse + TCP connection setup.

A simple fix is to re-check the map after acquiring the write lock:

♻️ Proposed fix

 	// Create new client (outside lock to avoid holding lock during slow operation)
 	clientset, err := cm.createHostedClusterClient(ctx, namespace, name)
 	if err != nil {
 		return nil, err
 	}

 	// Cache the client with write lock
 	cm.mu.Lock()
+	// Re-check: another goroutine may have populated the cache while we were creating
+	if existing, ok := cm.hcClients[key]; ok {
+		cm.mu.Unlock()
+		return existing, nil
+	}
 	cm.hcClients[key] = clientset
 	cm.mu.Unlock()

 	return clientset, nil

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/client.go` around lines 56 - 79,
GetHostedClusterClient uses double-checked locking and can create duplicate
clients: after creating a client with createHostedClusterClient and before
storing it into cm.hcClients, re-acquire the write lock (cm.mu.Lock) and
re-check cm.hcClients[key]; if an entry now exists, return that existing client
and dispose/close the newly created one (if it supports closing) instead of
unconditionally overwriting; otherwise store the newly created client and return
it. Ensure you use the same key computed in GetHostedClusterClient and the same
lock fields cm.mu and map cm.hcClients in the fix.

---

172-205: Remove unused ctx and mgmtClient parameters from replaceServerWithInternalEndpoint.

The function performs pure in-memory kubeconfig transformation and does not reference either parameter. Removing them simplifies the signature and clarifies that no I/O is performed.

♻️ Proposed fix

-func replaceServerWithInternalEndpoint(ctx context.Context, mgmtClient client.Client, kubeconfig *clientcmdapi.Config, hostedClusterNamespace, hostedClusterName string) error {
+func replaceServerWithInternalEndpoint(kubeconfig *clientcmdapi.Config, hostedClusterNamespace, hostedClusterName string) error {

Update the call site at line 107:

-	if err := replaceServerWithInternalEndpoint(ctx, cm.mgmtClient, kubeconfig, namespace, name); err != nil {
+	if err := replaceServerWithInternalEndpoint(kubeconfig, namespace, name); err != nil {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/client.go` around lines 172 - 205, The
function replaceServerWithInternalEndpoint has unused parameters ctx and
mgmtClient; remove those parameters from its signature and definition so it only
accepts (kubeconfig *clientcmdapi.Config, hostedClusterNamespace,
hostedClusterName string) (or reorder to match current usage), update all call
sites to stop passing a context or management client, and run tests/compile to
ensure no remaining references to replaceServerWithInternalEndpoint(ctx,
mgmtClient, ...) remain.

internal/controller/csrapproval/controller.go (1)

101-107: Both controllers watch the same resource type — verify no event contention.

Both dpfhcpprovisioner and csrapproval controllers use For(&provisioningv1alpha1.DPFHCPProvisioner{}). This is supported by controller-runtime (each controller gets its own work queue), but every status update from either controller will trigger reconciliation in both controllers. Since the CSR approval controller updates the CSRAutoApprovalActive condition on the same CR, this creates a feedback loop where each CSR approval status update re-triggers both controllers.

This is functionally safe (the provisioner controller will no-op if nothing changed, and the CSR controller will just re-poll), but it doubles the reconciliation rate. Worth being aware of for observability/log noise at scale.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/controller.go` around lines 101 - 107, The
two controllers both watch DPFHCPProvisioner and status-only updates cause both
to reconcile; update CSRApprovalReconciler's SetupWithManager to add an event
filter so status-only changes (like CSRAutoApprovalActive flips) don't trigger
reconciliation for the other controller — for example replace the current
ctrl.NewControllerManagedBy(mgr).For(...) chain in SetupWithManager with one
that calls WithEventFilter(predicate.GenerationChangedPredicate{}) or a custom
predicate (predicate.Funcs) that ignores updates where only the
status/conditions changed, or that only requeues when the CSRAutoApprovalActive
condition itself changes; modify the SetupWithManager function in
CSRApprovalReconciler accordingly.

internal/controller/csrapproval/csrapproval.go (2)

107-113: Consider a distinct Reason for the transient-error condition state.

Setting ConditionTrue with ReasonCSRApprovalActive when processPendingCSRs returns an error conflates a degraded state with a healthy one. Alerting/monitoring rules keyed on CSRAutoApprovalActive=True will silently miss this degraded path. A separate reason (e.g., ReasonCSRApprovalDegraded) would let operators distinguish "active and healthy" from "active but encountering errors."

♻️ Proposed change

-		if condErr := a.setCondition(ctx, dpfhcp, metav1.ConditionTrue, provisioningv1alpha1.ReasonCSRApprovalActive,
-			fmt.Sprintf("CSR processing encountered transient error: %v", err)); condErr != nil {
+		if condErr := a.setCondition(ctx, dpfhcp, metav1.ConditionTrue, provisioningv1alpha1.ReasonCSRApprovalDegraded,
+			fmt.Sprintf("CSR processing encountered transient error: %v", err)); condErr != nil {

(Add ReasonCSRApprovalDegraded to the API constants.)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/csrapproval.go` around lines 107 - 113, The
current code marks the condition as ConditionTrue with ReasonCSRApprovalActive
even when processPendingCSRs returns an error; add a new reason constant (e.g.,
ReasonCSRApprovalDegraded) to the API constants and use it in the error branch
so setCondition(ctx, dpfhcp, metav1.ConditionTrue, ReasonCSRApprovalDegraded,
fmt.Sprintf(...)) is called when err != nil (leave the existing
ReasonCSRApprovalActive for the successful/healthy path); update any
references/tests to the reason name accordingly so operators can distinguish
active+healthy vs active+degraded.

---

153-156: Consider filtering pending CSRs server-side to reduce per-poll data transfer.

On every 30-second poll all CSRs are fetched and filtered in-memory. For hosted clusters with many historical (approved/denied) CSRs, this is wasteful. A field selector or label selector at the API level would reduce the payload.

♻️ Example — field selector for unapproved CSRs

-	csrList, err := hcClient.CertificatesV1().CertificateSigningRequests().List(ctx, metav1.ListOptions{})
+	// Filter server-side to CSRs that have not yet been approved/denied
+	csrList, err := hcClient.CertificatesV1().CertificateSigningRequests().List(ctx, metav1.ListOptions{
+		// Note: Kubernetes does not support status-based field selectors on CSRs natively;
+		// use a label convention on bootstrap/serving CSRs if available, or keep in-memory filter
+		// but at minimum set a ResourceVersion to avoid unnecessary full re-lists.
+		ResourceVersion: "0", // return from cache
+	})

(The most practical improvement is to add ResourceVersion: "0" so the apiserver can serve from its watch cache rather than etcd on every poll.)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/controller/csrapproval/csrapproval.go` around lines 153 - 156,
Currently all CSRs are fetched then filtered in-memory; change the List call to
request only pending CSRs and use the watch cache by passing a ListOptions with
a server-side selector and ResourceVersion "0". Update the
hcClient.CertificatesV1().CertificateSigningRequests().List(ctx,
metav1.ListOptions{}) invocation to pass metav1.ListOptions{FieldSelector:
"<selector-for-pending-csrs>", ResourceVersion: "0"} (or a label selector if you
add a label), so the API returns only unapproved/undecided CSRs and uses the
apiserver cache; keep the rest of the code that processes csrList unchanged.